Avoid no-op EC KV reads in post-send pull sync - #900

Summary

Solid, well-scoped implementation of the #880 contract: the marker protocol is cryptographically careful, the preload gate enumerates its consumers explicitly instead of guessing, and the fallbacks all fail toward the KV path. One blocking issue: the withdrawal path emits the marker-expiry Set-Cookie unconditionally, which downgrades cache privacy for an entire traffic class that previously had no Set-Cookie at all.

2 of the inline comments below carry a one-click GitHub suggestion — use Commit suggestion (or Add suggestion to batch) to apply them as commits on the PR branch. Both were applied in an isolated worktree at this head and verified with cargo fmt --all -- --check, cargo clippy-fastly, cargo check-axum, cargo check-cloudflare, and the native trusted-server-core ec:: + publisher marker tests (332 and 28 passing respectively), with a byte-exact pre/post drift check. The remaining comments describe the fix in prose because it spans two files.

Blocking

🔧 wrench

• Withdrawal always emits a marker-expiry Set-Cookie, downgrading cache privacy for all withdrawn traffic — see inline at crates/trusted-server-core/src/ec/finalize.rs:63

Non-blocking

🤔 thinking

• Marker issuance re-adds a Set-Cookie to responses PR #885 deliberately made cookie-free — see inline at crates/trusted-server-core/src/ec/finalize.rs:160

♻️ refactor

• Completeness duplicates dispatch eligibility; the two can drift — see inline at crates/trusted-server-core/src/ec/pull_sync_marker.rs:170

⛏ nitpick

• The three-state loop asserts the same thing three times — see inline at crates/trusted-server-adapter-fastly/src/main.rs:553

👍 praise

• Marker crypto and framing — see inline at crates/trusted-server-core/src/ec/pull_sync_marker.rs:299

Cross-cutting / body-level findings

• 📝 Verified as correct, for the record — several things that look risky on first read hold up: the expect in reconcile_marker is unreachable because the belongs_to check precedes the Present match and entry_for ignores generation; the partner-set fingerprint is consistent across the validate and issue sites because every caller builds the registry from PartnerRegistry::from_config(&settings.ec.partners); upsert_partner_ids_from_snapshot returns early on empty updates, so a marker-valid navigation really does perform zero reads rather than moving the read later; stored KV EIDs reach only the auction path, which auction_needs_row forces the read for; tombstones are excluded via consent.ok; and per-partner pull_sync_ttl_sec is not wired into eligibility, so presence-only completeness matches dispatch today (see the ♻️ finding for the drift risk that creates).

CI Status

• cargo fmt: PASS
• cargo test: PASS
• cargo test (axum native): PASS
• cargo test (cloudflare native + wasm32-unknown-unknown): PASS
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• cargo test (cross-adapter parity): PASS
• cargo test (ts CLI, native): PASS
• vitest: PASS
• format-typescript: PASS
• format-docs: PASS
• prepare integration artifacts: PASS
• integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• browser integration tests: PASS

All 14 reported checks pass. gh pr checks --required returned no names for this PR, so no check is annotated as branch-protection-required (the base branch is fix/kv-eid-request-snapshot-ec-ttl, not main).

Summary

Adds a signed, host-only browser marker that lets an eligible navigation skip the EC identity-graph read when pull-sync completeness is the only reason to inspect the row, and returns from post-send pull sync before the graph is constructed when there is nothing to pull. The design is sound and the skip is well fenced: I traced each path rather than only reading the diff.

4 of the inline comments below carry a one-click GitHub suggestion — use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. All four were applied and verified in an isolated worktree at 963c331 (cargo fmt --all -- --check, cargo clippy-fastly, cargo check-fastly, and cargo test -p trusted-server-core --lib ec:: — 334 passed), individually and as a batch, with a byte-exact pre/post drift check.

What I verified beyond the diff

• marker_valid implies a non-empty registry. partner_set_fingerprint returns None for an empty pull set, so a valid marker requires at least one pull-enabled partner, so auction_needs_row == should_run_auction. A marker skip can therefore never suppress a read the auction needs.
• A generated EC never carries a valid marker. generate_if_needed and replace_with_generated both call invalidate_for_replaced_ec(), so the generated-cookie path at ec/finalize.rs:150 always has a real snapshot. No phantom-cookie regression from entry_for(&ec_id).is_none().
• The EID gate matches its consumer exactly. collect_eid_cookie_updates reads only ts-eids and sharedId — the same two constants eid_cookie_may_need_persistence checks — and upsert_partner_ids_from_snapshot short-circuits on empty updates, so a NotRead snapshot costs no hidden read in finalize.
• No shared-cache leak from the new Set-Cookie. enforce_set_cookie_cache_privacy runs in apply_terminal_response_effects, i.e. after ec_finalize_response, and the template cache stores reader-neutral bytes rather than response headers.
• The expect() in reconcile_marker cannot fire. The !belongs_to(ec_id) early return plus the Present match arm together guarantee entry_for is Some.
• reconcile_marker runs exactly once per finalize call. Every branch either returns before it or falls through to a single call, so no response can pick up a duplicate marker Set-Cookie.
• Only the Fastly adapter calls build_pull_sync_context; the Cloudflare, Spin, and Axum adapters are unaffected by the signature change.

Non-blocking

♻️ refactor

• build_pull_sync_context doc no longer describes its return contract — see inline at crates/trusted-server-core/src/ec/pull_sync.rs:60
• entry_is_pull_complete doc omits the consent and empty-registry conditions — see inline at crates/trusted-server-core/src/ec/pull_sync.rs:285

🤔 thinking

• No test for the two "steady state implies no Set-Cookie" invariants — see inline at crates/trusted-server-core/src/ec/pull_sync_marker.rs:580
• should_preload_ec is double-duty and its safety argument is implicit — see inline at crates/trusted-server-core/src/publisher.rs:4411

Cross-cutting / body-level findings

• 🤔 prepare_pull_sync_after_send's test asserts a property that already held — crates/trusted-server-adapter-fastly/src/main.rs:553. pull_sync_noop_states_skip_post_send_graph_factory proves that a None plan does not build the graph, but the pre-PR shape (if is_real_browser && let Some(context) = build_pull_sync_context(..) wrapping run_pull_sync_after_send, which built the graph inside) already guaranteed that. The behaviour that actually changed — build_pull_sync_context returning None for a complete row or an empty pull registry — is covered by build_pull_sync_context_skips_empty_registry_and_complete_snapshot. The case this helper uniquely adds and does not cover is Some(context) with a failing graph_factory. Worth either covering that arm or noting that the helper exists purely to make the ordering testable.

• 🌱 Allocation churn on the finalize path — sorted_pull_partner_domains clones every source_domain String, and a single reconcile_marker walks it twice on the issue path (once directly, once through create_marker -> partner_set_fingerprint), with entry_is_pull_complete calling pull_enabled_partners() again. That is three to four partner-set traversals per response in Wasm, where CLAUDE.md asks to minimise allocations. Small in absolute terms and it does not belong in this PR, but a cached fingerprint on PartnerRegistry would remove all of it. Follow-up.

• 📝 Orphan-recovery detection is deferred by up to MARKER_MAX_AGE_SECS — a marker-skipped navigation never observes a Missing snapshot, so recovery_eligible cannot fire until the marker expires. This is explicitly accepted in the plan doc and docs/guide/edge-cookies.md; recording it here so the tradeoff is visible on the PR thread rather than only in the plan.

• 👍 The crypto hygiene here is genuinely good. Key separation through MARKER_KEY_LABEL rather than using ec.passphrase as the HMAC key directly; length-prefixed domain hashing in partner_set_fingerprint, which removes the concatenation ambiguity a naive join would introduce; constant-time verify_slice for the tag; a both-sided expiry window (expires_at <= now and expires_at > now + MARKER_MAX_AGE_SECS) so a forged far-future expiry is rejected; and Redacted plus a hand-written Debug so the marker cannot leak into logs. EcSnapshotPreloadInput naming all four "another consumer needs the row" guards is the right call too — it turns an implicit condition into something reviewable.

CI Status

• cargo fmt: PASS
• cargo test: PASS
• cargo test (axum native): PASS
• cargo test (cross-adapter parity): PASS
• cargo test (ts CLI, native): PASS
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• browser integration tests: PASS
• prepare integration artifacts: PASS
• vitest: PASS
• format-typescript: PASS
• format-docs: PASS

No check reported as required by branch protection on this branch; all 14 reported checks pass.

Summary

Adds a signed, one-hour browser marker asserting that the EC row already holds every pull-partner UID, and uses it to skip the KV snapshot preload and the post-send identity-graph construction. The cryptographic construction is solid — a proper two-level HMAC KDF with domain separation, constant-time tag verification, length-prefixed fingerprint hashing, a host-only Secure/HttpOnly cookie, and every rejection path failing closed into a full KV read. I found no forgery, replay, or injection vector.

The blocking findings are about behavior and payoff rather than crypto: orphan recovery stops firing on the skip path (a regression against main, and one the PR description says is preserved), and the auction_needs_row hatch cancels the skip on exactly the ad-serving navigations the change targets. Both were verified with scratch tests in a worktree at this head, not by inspection alone.

None of the inline comments below carry a one-click suggestion. Scratch verification demoted both suggestion candidates to prose: the clock-skew fix breaks an existing assertion in a different range, and the cookie-bound fix needs a shared constant that lives outside the hunk. Each comment gives the proposed code in a plain block to apply manually.

Blocking

🔧 wrench

• Orphan recovery silently stops firing on the marker-skip path — see inline at crates/trusted-server-core/src/ec/finalize.rs:97
• The optimization does not fire on ad-serving navigations — see inline at crates/trusted-server-core/src/publisher.rs:4417

❓ question

• What is the measured KV-read reduction in production? — see Cross-cutting below

Non-blocking

🤔 thinking / ♻️ refactor / ⛏ nitpick

• Zero clock-skew tolerance on the marker expiry upper bound — see inline at crates/trusted-server-core/src/ec/pull_sync_marker.rs:263
• Host-only cookie scope is an unstated security invariant — see inline at crates/trusted-server-core/src/ec/pull_sync_marker.rs:196
• Marker cookie is stored unvalidated at the parse site — see inline at crates/trusted-server-core/src/ec/mod.rs:181
• snapshot_already_read is constant-false at the call site — see inline at crates/trusted-server-core/src/publisher.rs:4424

Cross-cutting / body-level findings

• ❓ What is the measured KV-read reduction in production? — Findings 1 and 2 together mean the change trades an orphan-recovery regression for a benefit that, in the tests as written, only materialises on navigations with no matched ad slots. Before merging ~1,100 lines and a new signed browser cookie, could you share the measured before/after KV read counts on real traffic?

  Note that the pull-sync half of this PR stands on its own and looks right independently: the entry_is_pull_complete short-circuit in build_pull_sync_context (crates/trusted-server-core/src/ec/pull_sync.rs:76-81) plus prepare_pull_sync_after_send (crates/trusted-server-adapter-fastly/src/main.rs:355) already skip the post-send graph construction when there is nothing to pull, with no cookie, no HMAC, and no snapshot-skip involved. If the marker's measured win is small, closing #880 with just that part would avoid both blocking findings entirely.

• 📝 Verification performed for this review — Ran in a clean worktree at 4bd7adad: cargo fmt --all -- --check (clean), cargo clippy-fastly, cargo clippy-axum, cargo test-axum (26 passed), cargo test -p trusted-server-core --lib (2687 passed). Two scratch reproductions confirmed findings 1 and 2; both were reverted and the worktree restored to the PR head.

• 📝 Angles checked and cleared — For the record, these were investigated and found sound, so they need no action: the expect() at pull_sync_marker.rs:135 is provably unreachable (the guards at lines 121 and 126 make entry_for total on that arm); the three HmacSha256::new_from_slice(...).expect(...) calls cannot fail (HMAC accepts any key length, and a ≥32-character passphrase is separately enforced at config load in settings.rs:611-631); the \0-separated marker payload is not ambiguous because ec_id is is_valid_ec_id-filtered before it reaches ec_value; the non-constant-time fingerprint comparison before verify_slice leaks only a public config digest, not keying material; no new dependencies were added (hex, hmac, sha2 were already present); and the doc's cache-privacy claim is accurate — enforce_set_cookie_cache_privacy does downgrade any Set-Cookie-bearing response.

CI Status

• cargo fmt: PASS (required)
• cargo test: PASS (required)
• format-typescript: PASS (required)
• format-docs: PASS (required)
• cargo test (axum native): PASS
• cargo test (cloudflare native + wasm32-unknown-unknown) / cargo check: PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• cargo test (cross-adapter parity): PASS
• cargo test (ts CLI, native): PASS
• vitest: PASS
• prepare integration artifacts: PASS
• Analyze (javascript-typescript): PASS
• Analyze (actions): PASS
• Analyze (rust): PENDING
• integration tests: PENDING
• integration tests (Fastly EC lifecycle): PENDING
• browser integration tests: PENDING
• CodeQL: SKIPPED

No failing checks at review time.